fix: improve language selection resolution #1011
Conversation
achou11
force-pushed
the
978/language-preference-resolution
branch
from
2ac269d to
50de297
Compare
achou11
force-pushed
the
978/language-preference-resolution
branch
from
50de297 to
71fb95f
Compare
achou11
force-pushed
the
978/language-preference-resolution
branch
from
71fb95f to
3a244a6
Compare
|
|
||
| import {MMKVZustandStorage} from '../hooks/persistedState/createPersistedState'; | ||
|
|
||
| export const STORAGE_KEY = 'MapeoLocale'; |
There was a problem hiding this comment.
just noting that this is the same storage key that is currently used for persisting the locale in the app.
| migrate: (persistedState, version) => { | ||
| /** | ||
| * Version 0 stores the state as `{ locale: string, setLocale: (locale: string) => void }`. | ||
| * We only need to handle the `locale` field, which is more specifically a language tag. | ||
| */ | ||
| if (version === 0) { | ||
| // Ensure that the persisted state for version has expected shape before attempting to migrate | ||
| if ( | ||
| typeof persistedState === 'object' && | ||
| persistedState !== null && | ||
| 'locale' in persistedState && | ||
| typeof persistedState.locale === 'string' | ||
| ) { | ||
| // TODO: log to Sentry to help understand how often this is happening? | ||
| return {languageTag: persistedState.locale}; | ||
| } | ||
| } | ||
|
|
||
| return {languageTag: null}; | ||
| }, |
There was a problem hiding this comment.
Highlighting this migration implementation since it's important to confirm that it's doing the right thing (since it's kind of hard to test...).
also would help to get some input on whether to log to sentry (see todo comments here)
There was a problem hiding this comment.
i have an idea of how to go about writing tests for this, but would require a decent chunk of additional work I think. might wait for input before trying.
| @@ -17,7 +17,7 @@ type PersistedStoreKey = | |||
| | 'ActiveProjectId' | |||
| | 'Settings' | |||
| | 'MetricDiagnosticsPermission'; | |||
| const MMKVZustandStorage: StateStorage = { | |||
| export const MMKVZustandStorage: StateStorage = { | |||
There was a problem hiding this comment.
probably should either:
- do some work to clean up how we're setting up the persistence stuff
- move this variable elsewhere that feels easier to discover
kind of taking the lazy route for now
| }); | ||
| }); | ||
|
|
||
| // TODO: Not sure if the truncation is a desired outcome, but it matches pre-existing behavior |
There was a problem hiding this comment.
just making a note of this. i found it kind of interesting that we only keep the language code as opposed to the whole tag. think the consequences are pretty minimal for now, but it does have potential consequences if we wanted to cater to more specific variants of a language
There was a problem hiding this comment.
not super happy with how access to the locale-related info is done here but felt the least invasive compared to defining some constructor opt and piping a callback to get access to the locale storage
| // Not strictly necessary to create this but helpful to make the tests a bit more descriptive | ||
| function setSystemPreferredLocales(locales: string[]) { | ||
| jest.mocked(useLocales).mockReturnValue(locales.map(mockLocale)); | ||
| } |
There was a problem hiding this comment.
there's probably a better way of going about this by mocking at a deeper level in expo-localization, but i think this is good enough for now.
| if (!stored) { | ||
| // TODO: Log to Sentry? | ||
| return null; | ||
| } |
There was a problem hiding this comment.
wondering if it'd be worth logging to sentry here because it means that the storage entry doesn't exist at all, which is unexpected
| return parsed.languageTag; | ||
| } | ||
|
|
||
| // TODO: Log to Sentry? |
There was a problem hiding this comment.
wondering if we should log to sentry here because it means that the stored value does not have the expected field we want to extract
There was a problem hiding this comment.
Wow!
What a well written PR.
Very nice and thorough tests!
I think the single hook for usage in the components was nice and clear.
I tested everything out using an emulator and device in as many ways as I could think of and everything seemed to work as expected.
I did not test out the migration... Or maybe I did? Not sure exactly how? I did install the develop version of the app on a device and set the system language to Spanish, which did nothing... Then updated to this PR without clearing anything or deleting the app. And then when the app reloaded it had selected Spanish. Not sure if that was a migration or the new set language action.
Thanks for explaining these: “locale” vs. “language tag” vs. “language code”
I wonder if this documentation, which is very thorough and clear and you seem to have went to a great deal of trouble to create, should exist somewhere besides this PR?
I think this is more related to the original issue. It seems to confirm that this PR is addressing the issue where we previously don't respect system preferences. The migration I'm referring to is the migration of the stored value via zustand's persistence plugin (which uses mmkv underneath) before and after this new implementation is used. I think doing the following steps could confirm that the migration works:
It's a bit tedious and admittedly have not tried this myself yet, but I think this would be enough to know that the zustand migration works as expected. |
Probably would make sense to. Don't really have a strong idea of where but maybe a |
@achou11 |
I think what you tried reflects what I suggested, which is unfortunate for me 😅 will also try it myself but seems like i'll have to take a closer look at the migration code and add tests |
|
Moving this to a draft in light of #1018, as I think we'll want to update this PR to build on top of the work to address that issue |
Fixes #978
The issues
There were a few issues I noticed about how we handle language settings that were prone to bugs:
We immediately persist the initial state of a persisted zustand store. It wasn't apparent to me at the time of implementation, but this is not a desirable behavior. Persistence should generally happen based on explicitly actions or via some kind of clearly defined effect. Doing this implicitly contributes to the issue that is detailed next.
System preferences could be ignored unintentionally. Basically what's highlighted in App does not use system language preference when expected #978, but here's a sequence that highlights the various issues:
Steps (4) and (5) highlight a couple of the issues:
The main problem here lies in the first issue I mentioned. Using a more code-friendly explanation, what's happening is the following:
'en'as the language to use for the app.'en'is persisted to storage because of issue 1'pt', but our calculation of the language to use is not reactive to this, as we only check the system settings when initializing Zustand (usinggetLocales()fromexpo-localization) and only in the case that something is not already persisted'en'even though it should be'pt'Terminology
Before getting into the changes that address the issues above, it's helpful to clarify some wording that I've committed to for this PR. I found it confusing to figure out what a "locale" meant versus a "language code" versus a "language tag" when it came to what's represented in code, so I made a concerted effort to distinguish between them appropriately:
locale: kind of this nebulous definition of a language based on many components that define how a language is presented and used. basically, the
Localetype fromexpo-localizationis a good example. given this, I found our usage of the wordlocalein the codebase to be a bit too vague (and somewhat misleading), so I made lots of efforts in the PR to avoid using it for naming where possible.language tag: basically what's detailed here. These can either be single or multi-component strings, where each components adds a degree of specificity (e.g. regional code). For example,
enanden-USare both language tags.language code: in practice, it's the component of a language tag that defines the primary language, which is typically the first component of the language tag (i.e. before the first hyphen if there are any). For example, the language code for
en-USisen. For all intents and purposes of our usage, every language code is a valid language tag.Changes this PR introduces
uses our new approach to Zustand-based state for persisting a selected locale. What's persisted is information related to language that is explicitly selected via user action (e.g. a store action). In this case, the storage looks like
{ languageCode: string | null }. There is no more implicit persistence on initialization like before.introduces a hook called
useResolvedLanguageTag()which figures out which language tag to use for the app based on:This hook solves the issue where we aren't reactively updating based on changes to the above. In addition to that, it also accounts for multiple system preferences. Before, we'd only take the first language preference and use that, but that's prone to unideal behavior because a user could have a set of preferred languages that may not actually be supported by the app. For example, if the system preferences were something like
["foo", "bar", "pt"], the existing behavior would attempt to only use"foo"despite"pt"being available and supported in the app.cleans up the implementation for intl-related utilities and resolution of language tags. I noticed that there's a noticeable distinction between the following:
messages/)languages.jsonfile.Using this categorization, the
useResolvedLanguageTag()hook basically returns the best fitting "usable" languageSome implementation concerns/questions
Use existing storage key or not?
As I refactored the locale state setup, I was trying to decide between two options:
usePersistedLocale()hook implementation)At the time of writing this, I've currently chosen (1) for the sake of backwards compatibility. (2) would be easier and less work, but in the worst case, it would mean that anyone who did explicitly select a language before would lose that selection. The app would be presented based on the system preference or the English fallback. Personally I don't find this to be a major issue but I can imagine it being quite surprising, especially for low tech users.
In choosing (1), I've updated the shape of the storage to be more forwards compatible. There's a zustand migration that i've implemented to migrate from version 0 (the default version associated with previous implementation) and version 1 (this new implementation). You may think this migration implementation is unnecessary work. In terms of what's stored, that's a valid criticism, but I would argue that we should be getting in the habit of versioning all of our persisted stores and implementing migrations where appropriate (such is the consequence of offline + local first!)
I'm not too sure what the best way to test the migrations is, as it ideally requires having the previous storage version persisted and then updating the app to use the new one in order to trigger the migration. Open to thoughts on how to go about that in a somewhat reliable and reproducible way.
Example of the app responding to changes in system preferences (no explicit choosing of language in in-app settings):
system-language-switch.mp4